Store PasswordExpiry and OAuthRefreshToken #1464

hickford · 2023-11-03T19:48:43Z

Add properties ICredential.PasswordExpiry and ICredential.OAuthRefreshToken. These correspond to Git credential attributes password_expiry_utc and oauth_refresh_token, see https://git-scm.com/docs/git-credential#IOFMT. Previously these attributes were silently disarded.

Plumb these properties from input to host provider to credential store to output.

Credential store support for these attributes is optional, marked by new properties CredentialStore.CanStorePasswordExpiry and ICredentialStore.CanStoreOAuthRefreshToken. Implement support in CredentialCacheStore, SecretServiceCollection and WindowsCredentialManager.

Add method IHostProvider.ValidateCredentialAsync. The default implementation simply checks expiry. Other implementations might query a server.

Improve implementations of GenericHostProvider and GitLabHostProvider. Previously, GetCredentialAsync saved credentials as a side effect. This is no longer necessary. The workaround to store OAuth refresh tokens under a separate service is no longer necessary assuming CredentialStore.CanStoreOAuthRefreshToken. Querying GitLab to check token expiration is no longer necessary assuming CredentialStore.CanStorePasswordExpiry.

Fixes #1463
Fixes #268
Fixes #2059

Add properties ICredential.PasswordExpiryUTC and ICredential.OAuthRefreshToken. These correspond to Git credential attributes password_expiry_utc and oauth_refresh_token, see https://git-scm.com/docs/git-credential#IOFMT. Previously these attributes were silently disarded. Plumb these properties from input to host provider to credential store to output. Credential store support for these attributes is optional, marked by new properties ICredentialStore.CanStorePasswordExpiryUTC and ICredentialStore.CanStoreOAuthRefreshToken. Implement support in CredentialCacheStore, SecretServiceCollection and WindowsCredentialManager. Add method IHostProvider.ValidateCredentialAsync. The default implementation simply checks expiry. Improve implementations of GenericHostProvider and GitLabHostProvider. Previously, GetCredentialAsync saved credentials as a side effect. This is no longer necessary. The workaround to store OAuth refresh tokens under a separate service is no longer necessary assuming CredentialStore.CanStoreOAuthRefreshToken. Querying GitLab to check token expiration is no longer necessary assuming CredentialStore.CanStorePasswordExpiryUTC.

hickford · 2024-02-13T20:54:44Z

Windows build ought to be fixed by #1418

ldennington

Overall looks good! Most of my comments are small things - the only major issue that I'd need to see updated before approving is not having expiry implemented for Windows Credential Manager.

src/shared/Core/Credential.cs

src/shared/Core/CredentialCacheStore.cs

src/shared/Core/Interop/Windows/WindowsCredentialManager.cs

src/shared/GitLab/GitLabHostProvider.cs

src/shared/TestInfrastructure/Objects/TestCredentialStore.cs

src/shared/Core/Interop/Windows/WindowsCredential.cs

ldennington

Thank you for the updates!

src/shared/Core/Interop/Windows/WindowsCredentialManager.cs

src/shared/Core/HostProvider.cs

Marshal.FreeHGlobal and Marshal.DestroyStructure

Limit iterations in GetCredentialAsync

PetSerAl

My opinion (feal free to disregard it): it is better not to remove refresh token unnecessary. I understand, that GCM can just reauthenticate, but that require extra, otherwise unnecessary, browser interaction from user, thus providing slightly worse user experience.

Anyways, I have no intentions to spend my time to pursue this. Error rate unlikely would be that hi to bother me that much.

hickford · 2025-02-26T21:31:34Z

@mjcheetham any time to look at this?

dscho · 2025-02-26T21:40:58Z

@mjcheetham any time to look at this?

@hickford: @mjcheetham is no longer working at GitHub (but is once again my teammate, this time at Microsoft). Git Credential Manager, however, is still owned by GitHub and it is currently not clear to me who at GitHub takes care of this here repository.

becm · 2025-03-16T16:16:44Z

src/shared/Core/GenericHostProvider.cs


                    // Return the new access token
-                    return new GitCredential(oauthUser,refreshResult.AccessToken);
+                    return new GitCredential(refreshResult, oauthUser);


Delaying the save of a refresh_token will likely result in issues.

New value must be saved, even if access_token is invalid for the requested operation.
This means we'd have to check (and rely on git) to also provide the new refresh_token and store its value during an erase callback (needs ugly hack in EraseCredentialAsync).

~~Granted the current state (without #1838) is also broken.~~ (fix has been merged)
I think this is still the correct location to directly issue a store.
Otherwise there is a risk of client desync due to missed refresh_token value updates.

There are advantages and disadvantages to pre-emptively storing the refresh token. In particular, storing the wrong username if the user has multiple accounts on the host and selects the wrong account. In general, I'd prefer to avoid the complexity of side effects. The Git credential API has deliberate independent verbs "get/store/erase".

Which host do you test with?

Chances are high the 1st action is a pull from a repository readable by the 2nd account.
So any existing entry is likely to be replaced anyway (no failure as account guard). ☺

Git can only give feedback (store vs. erase) on the access_token.
Any provided refresh_token is (by convention) valid and MUST be used in further interactions.
Saving that during the get phase is not a side effect but required as part of a completed transaction.
Delaying the save can break things on Ctrl+C, parallel pulls, or stupid proxies
(see comments regarding missing token updates).

The primitive get with detached store/erase is broken in the context of OAuth.
A 2nd parallel action from the same host will trigger another token refresh and a conflicting store
(not an issue to have multiple access_token, but potentially fatal with an outdated refresh_token) .

My primary endpoint is Forgejo with recommended single-use tokens (→ #1838 required).
Primary client is Win11 (where leaking refresh_token in the output would be only a minor violation).
(→ getting to the next potential problem with the get/store approach on a proper secret store…)

Okay -- I've restored that logic. Would you be able to test this branch with Forgejo?

becm · 2025-03-16T16:18:58Z

src/shared/Core.Tests/Commands/GetCommandTests.cs

-                ["password"] = testPassword
+                ["password"] = testPassword,
+                ["password_expiry_utc"] = testExpiry.ToString(),
+                ["oauth_refresh_token"] = testRefreshToken,


The refresh_token will likely also have an expiry date.
At least one observed implementation however does not report the constraint.

Any suggestion to improve the test?

Sorry, wrong/stupid location for my comment (test is not the issue). ☹
(Maybe you can mark this part of my comments as resolved to avoid further confusion?)

General problem with the auth flow when not dealing with refresh in the get phase:

presenting git with an expired credential will just lead to ignoring it

the refresh_token might also be expired (but git would have no way of recovering anyway…)

Telling git should only be useful if there was a further provider with valid credentials as a fallback…
But since GCM has no way of knowing this, it's likely best to keep handling refresh proactively in get.
Which would of course render the whole concept of supplying any token metadata to git redundant.

…fresh-token

Suggested-by: Marc Becker

becm · 2026-01-11T04:01:11Z

Hm, lumping together these 2 distinct entities opens a can of worms when trying to do updates consistently.

update of refresh_token can not be missed in any code path
erase may NOT invalidate the refresh_token part (as hinted by @PetSerAl)

So this would mean (to be consistent with current/fallback behavior):

during initial registration, a partial secret must already be constructed and saved (invalid access_token)
during refresh a partial update of the secret is required (refresh_token only)
the erase must be limited to the access_token part
(if refresh_token is the same value, else it came from a successful git call that was started later)

When updating the access_token part, the value for refresh_token MUST be read again,
it may have been updated in the time window between token refresh and token store by a parallel git call.

The whole approach of partial updates also massively increases potential for conflicting parallel operations.
Now: Each get needs consistent read/store
After: Each store/erase also has the potential to mess something up.
Not sure how often these conflicts appear in regular use, but the devil is in the details…
And each failure will lead to client invalidation in case of single-use refresh_token requirements.

becm · 2026-01-11T04:10:16Z

To be honest, in the OAuth case I would opt to

store the access_token during the get phase and
make store and erase a NOOP.

This is likely to reduce potential conflicts even in the current approach.
But there may be reservations to deviate even more from the classical git credential flow.
(Which in my opinion only fits well with a fully interactive flow, but not matching well with OAuth exchanges)

hickford force-pushed the expiry-and-oauth-refresh-token branch 3 times, most recently from a2425b9 to a21c066 Compare November 3, 2023 21:48

hickford changed the title ~~introduce PasswordExpiryUTC and OAuthRefreshToken~~ introduce PasswordExpiry and OAuthRefreshToken Nov 3, 2023

hickford force-pushed the expiry-and-oauth-refresh-token branch from a21c066 to 2a14b66 Compare November 4, 2023 06:36

hickford changed the title ~~introduce PasswordExpiry and OAuthRefreshToken~~ Store PasswordExpiry and OAuthRefreshToken Nov 10, 2023

hickford added the enhancement New feature or request label Nov 10, 2023

hickford marked this pull request as ready for review November 10, 2023 20:07

hickford force-pushed the expiry-and-oauth-refresh-token branch 2 times, most recently from 83c3f3a to b59547b Compare February 22, 2024 20:14

ldennington requested changes Apr 15, 2024

View reviewed changes

This comment was marked as spam.

Sign in to view

hickford force-pushed the expiry-and-oauth-refresh-token branch 4 times, most recently from 01fcd91 to fdd7759 Compare November 1, 2024 20:46

hickford requested review from ldennington and mjcheetham November 1, 2024 20:47

hickford force-pushed the expiry-and-oauth-refresh-token branch from fdd7759 to 60224e0 Compare December 6, 2024 19:35

ldennington approved these changes Dec 14, 2024

View reviewed changes

PetSerAl reviewed Jan 1, 2025

View reviewed changes

src/shared/Core/Interop/Windows/WindowsCredentialManager.cs Outdated Show resolved Hide resolved

src/shared/Core/Interop/Windows/WindowsCredentialManager.cs Outdated Show resolved Hide resolved

src/shared/Core/HostProvider.cs Outdated Show resolved Hide resolved

fixup! introduce PasswordExpiryUTC and OAuthRefreshToken

af784ad

Marshal.FreeHGlobal and Marshal.DestroyStructure

This comment has been minimized.

Sign in to view

fixup! introduce PasswordExpiryUTC and OAuthRefreshToken

e49fef0

Limit iterations in GetCredentialAsync

hickford requested a review from PetSerAl January 3, 2025 07:56

PetSerAl reviewed Jan 3, 2025

View reviewed changes

jonathanduke mentioned this pull request Feb 7, 2025

GCM OAuth Issue - GCM Erases Expired Token But Does Not Renew - Gitea #1408

Open

becm mentioned this pull request Feb 15, 2025

fix(generic): check local token expiry #1837

Open

This comment has been minimized.

Sign in to view

becm reviewed Mar 16, 2025

View reviewed changes

becm mentioned this pull request Apr 26, 2025

Install includes stale Git Credential Manager git-for-windows/git#5606

Open

Merge remote-tracking branch 'upstream/main' into expiry-and-oauth-re…

3873076

…fresh-token

hickford requested review from a team as code owners November 11, 2025 21:10

GenericHostProvider: immediately store refresh token

39ba57e

Suggested-by: Marc Becker

Store PasswordExpiry and OAuthRefreshToken #1464

Are you sure you want to change the base?

Store PasswordExpiry and OAuthRefreshToken #1464

Uh oh!

Conversation

hickford commented Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hickford commented Feb 13, 2024

Uh oh!

ldennington left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as spam.

Uh oh!

ldennington left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

PetSerAl left a comment

Choose a reason for hiding this comment

Uh oh!

hickford commented Feb 26, 2025

Uh oh!

This comment has been minimized.

dscho commented Feb 26, 2025

Uh oh!

This comment has been minimized.

becm Mar 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hickford Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

becm Jan 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hickford Jan 10, 2026

Choose a reason for hiding this comment

Uh oh!

becm Mar 16, 2025

Choose a reason for hiding this comment

Uh oh!

hickford Jan 10, 2026

Choose a reason for hiding this comment

Uh oh!

becm Jan 10, 2026

Choose a reason for hiding this comment

Uh oh!

becm commented Jan 11, 2026

Uh oh!

becm commented Jan 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

hickford commented Nov 3, 2023 •

edited

Loading

ldennington left a comment •

edited

Loading

becm Mar 16, 2025 •

edited

Loading

becm Jan 10, 2026 •

edited

Loading